Skip to content

Ignore expression when asserting that sorts are equal#1498

Closed
marko-bekhta wants to merge 1 commit into
jakartaee:mainfrom
marko-bekhta:fix/sort-assert
Closed

Ignore expression when asserting that sorts are equal#1498
marko-bekhta wants to merge 1 commit into
jakartaee:mainfrom
marko-bekhta:fix/sort-assert

Conversation

@marko-bekhta

Copy link
Copy Markdown
Member
  Expected :Sort[expression=null, property=hexadecimal, isAscending=true, ignoreCase=true, nullOrdering=UNSPECIFIED]
  Actual   :Sort[expression=TextAttributeRecord[name=hexadecimal], property=hexadecimal, isAscending=true, ignoreCase=true, nullOrdering=UNSPECIFIED]

Since string-based sorts don't get an expression ... but the ones created from attributes do ... let's just not expect them to match ?

cc: @njr-11

@njr-11 njr-11 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since string-based sorts don't get an expression ... but the ones created from attributes do ... let's just not expect them to match ?

It looks like you are recommending to have the TCK overlook the change in behavior by altering the test case. Even if the test case is made to conveniently not notice, Data 1.1 would still be introducing a behavior change vs Data 1.0. We cannot do that.

@marko-bekhta

Copy link
Copy Markdown
Member Author

I mean ...

default Sort<T> ascIgnoreCase() {
return Sort.ascIgnoreCase(this);
}

public static <T> Sort<T> ascIgnoreCase(String attribute) {
return new Sort<>(null, attribute, true, true, Nulls.UNSPECIFIED);
}

it's Data itself that broke it ?

@njr-11

njr-11 commented Jun 26, 2026

Copy link
Copy Markdown
Member

it's Data itself that broke it ?

Yes, and I was the one who broke it due to a mistake in one of my changes. I have a correction to what I broke in #1496 which has not been merged yet. We should be fixing the break rather than altering the TCK to overlook it.

@marko-bekhta

Copy link
Copy Markdown
Member Author

ah 🙂 so you already were working on a fix! Let's close this one then.
(note: we are currently running the snapshot version of this TCK as part of the Hibernate ORM build, hence we now notice such kind of failures in the build. Btw let me know if you'd want running that TCK integration as part of verifying your changes 🙂)

@njr-11

njr-11 commented Jun 29, 2026

Copy link
Copy Markdown
Member

(note: we are currently running the snapshot version of this TCK as part of the Hibernate ORM build, hence we now notice such kind of failures in the build. Btw let me know if you'd want running that TCK integration as part of verifying your changes 🙂)

I did already test #1496 by verifying the TCK passes again, but you are certainly welcome to test it with Hibernate. The only reason the fix wasn't merged is that it still needs a review by a Jakarta Data committer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants